Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rework glucose dialog #139

Closed
wants to merge 1 commit into from
Closed

Rework glucose dialog #139

wants to merge 1 commit into from

Conversation

BrianWieder
Copy link
Contributor

This ports Artificial-Pancreas/iAPS#272 into Open-iAPS, which replaces the manual glucose entry pop-up with a sheet.

This is part of #47

Simulator Screenshot - iPhone 15 - 2024-04-30 at 22 45 28

Simulator Screenshot - iPhone 15 - 2024-04-30 at 22 45 31

This ports Artificial-Pancreas/iAPS#272 into Open-iAPS.

This is part of #47

Co-Authored-By: Deniz Cengiz <48965855+dnzxy@users.noreply.github.com>
@bjornoleh
Copy link
Contributor

I tested the PR, and it works as intended.

However, it does not make sense to display mmol/L (or mg/dL) as header, and then repeat the units after each BG value. I am sure this is not what @dnzxy wanted (after reading the iAPS PR 272 discussions)

The minimal change that would make this look reasonable, is to replace the units in the header with "Glucose" (and make sure this is localised).

Screenshot from one of the suggestions in the iAPS PR:

image

Alternatively, only display units in the header, as was also presented in the other PR:

image

@dnzxy
Copy link
Contributor

dnzxy commented May 1, 2024

However, it does not make sense to display mmol/L (or mg/dL) as header, and then repeat the units after each BG value. I am sure this is not what @dnzxy wanted (after reading the iAPS PR 272 discussions)

The add glucose button belongs in the top right-hand corner of that view to adhere to iOS design guidelines. I had long discussions about this and could not get it in. Personally, I'm all for omitting the unit. It's clear from the app settings which unit was applied, so it is not necessary to display it in every row.

Personally, I also like the inverse (first value, then time), but that's a personal preference. It's how I have it in my personal build.

@bjornoleh
Copy link
Contributor

Thanks. I also notice that the unique ID is displayed for every reading, this is not meant for humans to read, and can safely be omitted too.

@BrianWieder
Copy link
Contributor Author

Ah now knowing its alright if we deviate a bit from the original PRs, I will make some further changes. I am going to close this PR and combine this and the port of Artificial-Pancreas/iAPS#314 into one PR.

@BrianWieder BrianWieder closed this May 2, 2024
@dnzxy dnzxy deleted the rework-glucose-dev branch May 14, 2024 08:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants